Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WIP: Multi-coins support for validators #4794

Closed
wants to merge 4 commits into from
Closed

WIP: Multi-coins support for validators #4794

wants to merge 4 commits into from

Conversation

RiccardoM
Copy link
Contributor

@RiccardoM RiccardoM commented Jul 29, 2019 •
edited by alexanderbez
Loading

Description

This PR should implement the support for multi-coins chains that want to allow validators to support multiple coins denominations during the related operations (delegation, rewards, etc.).

closes: #4783

Checklist

  • Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
  • Wrote tests
  • Updated relevant documentation (docs/)
  • Added a relevant changelog entry: clog add [section] [-t <tag>] [-m <msg>]
  • Re-reviewed Files changed in the github PR explorer

For Admin Use:

  • Added appropriate labels to PR (ex. wip, ready-for-review, docs)
  • Reviewers Assigned
  • Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

@RiccardoM RiccardoM mentioned this pull request Jul 29, 2019
Closed
4 tasks
fedekunze
fedekunze suggested changes Jul 29, 2019
View reviewed changes
Copy link
Collaborator

@fedekunze fedekunze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm against these changes. Extending x/staking to have multiple bond token denoms as this PR is doing is very general. This needs to be extremely specified on an ADR first, as these changes could have security implications.

@fedekunze fedekunze added the S:blocked Status: Blocked label Jul 29, 2019
@RiccardoM
Copy link
Contributor Author

@fedekunze Thank you for taking the time to help me suggesting what to do! Do you have an example of a previous ADR? I would love to take it as example in order to write the one for this implementation!

@fedekunze
Copy link
Collaborator

Sure @RiccardoM. Check here for the general structure. Previous ADRs are located here. There are other ADRs with PRs open, for eg #4732, #4737 and #4780

@alexanderbez
Copy link
Contributor

Why don't we just use sdk.Coin instead of multiple coins?

@RiccardoM
Copy link
Contributor Author

@alexanderbez I thought that since I was editing that code, I could implement multiple coins. The single coin will then just be a trivial usage of the multiple ones.

@alexanderbez
Copy link
Contributor

@alexanderbez I thought that since I was editing that code, I could implement multiple coins. The single coin will then just be a trivial usage of the multiple ones.

I don't imagine we'll support staking multi-coins anytime soon.

@fedekunze
Copy link
Collaborator

The single coin will then just be a trivial usage of the multiple ones.

You'd have to account for minting (which od the staking coins will you mint every block)? Same applies to slashing. Also, I don't know if tendermint supports multiple coins?

@alexanderbez
Copy link
Contributor

Tendermint only cares about power.

@stale
Copy link

stale bot commented Sep 30, 2019

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Sep 30, 2019
@stale stale bot closed this Oct 10, 2019
@RiccardoM RiccardoM deleted the riccardom/multi-coins-support branch October 16, 2019 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fedekunze fedekunze fedekunze requested changes

@alessio alessio Awaiting requested review from alessio

@alexanderbez alexanderbez Awaiting requested review from alexanderbez

@jackzampolin jackzampolin Awaiting requested review from jackzampolin

@rigelrozanski rigelrozanski Awaiting requested review from rigelrozanski

Assignees
No one assigned
Labels
S:blocked Status: Blocked
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Standardize tokens output across modules' REST APIs
3 participants
@RiccardoM @fedekunze @alexanderbez